-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use correct path for scaffolding spec on CT #21411
fix: use correct path for scaffolding spec on CT #21411
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment.
@@ -282,13 +282,15 @@ export class ProjectDataSource { | |||
} | |||
|
|||
async defaultSpecFileName () { | |||
const defaultFileName = 'cypress/e2e/filename.cy.js' | |||
const getDefaultFileName = (testingType: TestingType) => `cypress/${testingType}/filename.cy.${this.ctx.lifecycleManager.fileExtensionToUse}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem correct. What if cypress/component
does not exist? Considering the default is something like src/**/*.cy.{js.jsx}
, it likely won't exist for most users.
Should we make sure the default placeholder is a valid path (or we don't care?)
We could check against the specPattern to make sure it matches (we've already got this logic here.
If it does not match, we could do something like:
const root = specPattern.split('/')[0]
const getDefaultFileName = path.join(root, 'filename.cy', this.ctx.lifecycleManager.fileExtensionToUse)`
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
@@ -282,13 +282,15 @@ export class ProjectDataSource { | |||
} | |||
|
|||
async defaultSpecFileName () { | |||
const defaultFileName = 'cypress/e2e/filename.cy.js' | |||
const getDefaultFileName = (testingType: TestingType) => `cypress/${testingType}/filename.cy.${this.ctx.lifecycleManager.fileExtensionToUse}` | |||
|
|||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly related but do we need this try/catch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I think. The spec and pattern related logic is starting to get very confusing - I wonder if we should look to move it to either a) separate data source or b) separate module (or even just inside of @packages/config
.
I think data-context should just be a thin layer on how we access data and write data, not the place where we handle the complexity. I don't think we need to do that now, but we should keep it in mind as we start to transition from release mode to stability and maintainability mode.
|
||
if (!specFileName) { | ||
return defaultFileName | ||
} | ||
|
||
return specFileName | ||
} catch { | ||
return defaultFileName | ||
return getDefaultFileName('e2e') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we could just try/catch the awaited specPatterns call and abort on line 302 in the case of a failure and not need separate error logic here.
If we do need to keep this, though, why not use currentTestingType instead of 'e2e'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left the try/catch but updated the testing type - the reason is bc getDefaultSpecFileName
is using different packages and is safer to catch it if there's any possible error while trying to create the fileName depending of the specPattern
* 10.0-release: (22 commits) fix: migrate multiples projects when in global mode (#21458) test: fix flaky cy-in-cy selector validity test (#21360) chore: remove unused codeGenGlobs (#21438) fix: use correct path for scaffolding spec on CT (#21411) fix: remove breaking options from testing type on migration (#21437) fix: test-recording instructions in Component Test mode (#21422) feat: distinguish app vs launchpad utm_source when using utm params (#21424) chore: update stubbed cloud types (#21451) chore: change to yarn registry fix(sessions): refactor flows, fix grouping bugs and align validation fail text (#21379) chore(sessions): more driver tests (#21378) chore: rename domain_fn to origin_fn (#21413) chore: release 9.6.1 (#21404) fix: ensure that proxy logs are updated after the xhr has actually completed (#21373) chore: Re-organize tests in assertions_spec.js (#21283) chore: Distribute tests to desktop-gui containers. Make `desktop-gui` tests faster! (#21305) chore(sessions): add additional tests (#21338) fix: Allow submit button to be outside of the form for implicit submission (#21279) fix(launcher): support Firefox as a snap (#21328) chore(sessions): break out sessions manager code and add unit tests (#21268) ...
User facing changelog
We were scaffolding the CT specs on the root of
cypress
folder, in this case, if we identify that the specPatter used is the default one, we create the empty spec oncypress/component
Additional details
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?cypress.schema.json
?